Skip to content

feat(security): warn once when enableSecureText is disabled + document XSS trade-off#474

Closed
gnbm wants to merge 8 commits into
masterfrom
security/secure-text-warning-docs
Closed

feat(security): warn once when enableSecureText is disabled + document XSS trade-off#474
gnbm wants to merge 8 commits into
masterfrom
security/secure-text-warning-docs

Conversation

@gnbm

@gnbm gnbm commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • enableSecureText defaults to false, so option label/value are written to the DOM as raw HTML. Consumers rendering untrusted data without opting in are exposed to XSS, and there's no signal that this is happening.

  • Changing the default to true would be disruptive (breaks intentional-HTML consumers) and would impose per-option escaping cost on large datasets (10k–100k+ records) where it's often unnecessary. So per the agreed approach, this PR keeps the default off but makes the trade-off impossible to miss.

Change

  • One-time console warning: when options are rendered while enableSecureText is disabled, log a single (per page) console.warn pointing to the property docs. The check is O(1) — it never scans option content, so there's no cost on large datasets. Tracked by a static secureTextWarningShown flag.
  • Docs: a prominent security callout at the top of properties.md plus an expanded enableSecureText row explaining the XSS risk, when to enable it, and the large-dataset performance rationale for the default.

No behavioural/default change beyond the warning.

Verification

New spec cypress/e2e/secure-text-warning.cy.ts:

  • warns exactly once even when two instances initialise with enableSecureText off;
  • does not warn when enableSecureText: true.

Other Validations:

  • Ran npm run validate for static code validation: image
  • npm run build: succeeds; dist/docs rebuilt
  • All tests passed (ran npm run test ):
image

gnbm added 8 commits June 3, 2026 22:47
customData.group_name and customData.description were interpolated directly into
the option aria-label attribute without escaping. A double quote in those fields
could break out of the attribute and inject markup/handlers even when
enableSecureText was enabled - an XSS bypass.

Route both fields through secureText() (same path as label/value). secureText is
a no-op when enableSecureText is disabled, so behaviour is unchanged for
consumers that intentionally pass raw text.

Adds cypress regression spec security-customdata-xss.cy.ts.
… (S3)

Option classNames were concatenated into the class attribute unescaped, so a
consumer-provided value containing a double quote could break out of the attribute
and inject markup/handlers.

Add Utils.sanitizeClassNames() which strips the only characters that can break out
of a double-quoted attribute (" < >). Valid CSS class tokens never contain these,
so legitimate class names are unaffected (covered by a no-regression test).

Adds cypress regression spec security-classnames-xss.cy.ts.
…wrappers (P1)

The window 'resize' listener ran onResizeMethod on every tick, which queried all
.vscomp-ele-wrapper elements and recomputed each instance's options-container
height - O(instances) layout work per resize event during a drag. It also assumed
every wrapper's parent had a .virtualSelect and threw otherwise.

- Add Utils.throttle and wrap the resize listener (~100ms) so the per-instance
  work runs at most ~10x/sec.
- Guard onResizeMethod against wrappers whose instance is missing (teardown races).

Closed instances are still updated (opening does not recompute height, so skipping
them could leave a stale height on reopen) - throttling delivers the win without
that regression risk.

Adds cypress spec perf-resize-throttle.cy.ts (throttling, no-regression, guard).
…ent (S4/P3/P4)

P4: route fire-and-forget setTimeout calls through setManagedTimeout, whose ids are
tracked and cleared in destroy(). Prevents callbacks (e.g. renderOptions, focus)
from running against a destroyed instance.

P3: document the intentional forced reflow in openDropbox so it is not removed as a
no-op.

S4: document that the search-highlight regex relies on enableSecureText for escaping
and is not an additional injection vector (input is already regex-escaped).

Adds cypress spec timer-cleanup.cy.ts.
…ed + document XSS trade-off (S1)

enableSecureText is OFF by default and option label/value are rendered as raw HTML.
Rather than change the default (disruptive, and escaping is costly on large 10k-100k+
datasets), log a single per-page console.warn when options are rendered while
enableSecureText is disabled, so the XSS trade-off is discoverable. The check is O(1)
and never scans option content.

Also documents the trade-off loudly in docs/properties.md (top-of-page callout +
expanded enableSecureText row).

Adds cypress spec secure-text-warning.cy.ts.
Remove an unnecessary ts-expect-error in the timer cleanup test. In Utils.throttle, add JSDoc typings for timeout and lastArgs, initialize lastArgs as an array, annotate the throttled function args, and replace callback.apply(this, lastArgs) with callback(...lastArgs). These changes improve type-safety and simplify the callback invocation.
Add JSDoc type annotations for timeout and throttle args, initialize lastArgs as an array, and replace callback.apply(this, lastArgs) with callback(...lastArgs) to use spread invocation. Updated corresponding minified and docs assets (dist/virtual-select.js, dist/virtual-select.min.js, docs/assets/virtual-select.js, docs/assets/virtual-select.min.js). These changes improve type clarity and simplify argument handling in Utils.throttle.
@gnbm gnbm changed the base branch from chore/timer-cleanup-hardening to master June 9, 2026 23:04
@gnbm

gnbm commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator Author

Closing in favour of #480. Almost everything here is already on master: the managed-timeout cleanup (#477), the resize throttle — superseded by the ref-counted listener + cancel() impl (#476/#478) — and the reflow/highlight comments. This branch is CONFLICTING against master, and its module-level Utils.throttle(...) (no cancel, never removed) would regress master's teardown-aware handler. The single genuinely-new contribution — the one-time enableSecureText warning + properties.md security docs — has been rebased cleanly onto current master in #480 (validate + build clean; new spec passes; 16 existing security/perf/lifecycle tests still pass).

@gnbm gnbm closed this Jun 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant